fix(serializer): key the registry by Type so name-colliding packets serialize - #505
Conversation
…erialize The delegate dictionary was keyed by the simple class name; ten packets share a name across ClientPackets and ServerPackets namespaces, so one of each pair lost its registration and serializing it invoked the other type's delegate, throwing ArgumentException. Keying by Type gives every packet its own delegate and removes the collision handling outright. The corpus snapshot now pins all 398 packets with no exception entries, disambiguating colliding names by namespace, and is regenerated against current master - it also folds in the pstash header rename and the four newly headered packets that landed after the corpus PR branched, which is what currently fails CI on master. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe serializer now uses runtime ChangesPacket serializer type resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The serializer now keeps same-named packet types independent while preserving the public API, eliminating the reported collision failure. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The exceptions
Serializerkeyed_packetSerializerDictionarybytypeof(T).Name— the simple name. Ten packet class names exist in both aClientPackets.*and aServerPackets.*namespace;Initializelet the non-ClientPackets one win, so serializing the loser looked up the winner's compiled delegate andDynamicInvokethrewArgumentException. (Deserializernever had the problem — it keys by wire header.)The dictionary is private, so the fix keys it by
Type: every packet keeps its own delegate, the ClientPackets-wins eviction logic is deleted, and the failure mode is gone rather than pinned.Serializeand the injected-sub-packet path look up byGetType()directly (also faster than string hashing).Corpus/snapshot updates
OnlyTheKnownSimpleNameCollisionsFailToSerialize→EveryPacketSerializes: zero tolerated exceptions.CollidingPacketNamesAreStillDistinctTypesstays, now purely to catch new accidental duplicates.Also fixes master's red CI
#498 was merged without being up to date with master, and #500/#501 had changed six packets (
pstash/pstash_allheaders,DmPacket/EventPacket/FbtPacket/RcPacket) after its snapshot was generated — so the snapshot test currently fails on master. The regenerated snapshot here is taken against current master and folds those in. Verified: every changed snapshot line traces to either the collision fix or those two merged PRs; all other 780+ lines are byte-identical.127/127 tests pass locally.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests